[OPIK-347]: FE prompts list #546
Conversation
…o sasha/OPIK-347/prompts-list
| }: UsePromptCreateMutationParams) => { | ||
| const { data } = await api.post(PROMPTS_REST_ENDPOINT, { | ||
| ...prompt, | ||
| workspace_name: workspaceName, |
There was a problem hiding this comment.
I don't think this is needed, we're sending the comet-workspace request header on each request so the BE already knows the workspace without any extra parameter
It's okay to have it in the query key of react-query for invalidations since it's needed but no need to send it in the POST (probably copy/paste from old code where before the parameter was needed)
| return useMutation({ | ||
| mutationFn: async () => { | ||
| const { data } = await api.post(`${PROMPTS_REST_ENDPOINT}/versions`, { | ||
| ...prompt, |
|
|
||
| const PromptPage: React.FunctionComponent = () => { | ||
| const [tab, setTab] = useQueryParam("tab", StringParam, { | ||
| updateType: "replaceIn", |
There was a problem hiding this comment.
Not sure if replaceIn is the one here, probably the default pushIn (basically removing this third parameter) is okay (so back button in the browser will work), maybe in setting the default to prompt is okay to do setTab("prompt", "replaceIn")
@andriidudar what do you think?
| const { id: promptId, ...restPrompt } = prompt; | ||
|
|
||
| const { data } = await api.put(`${PROMPTS_REST_ENDPOINT}${promptId}`, { | ||
| ...restPrompt, |
| const { data } = await api.get(PROMPTS_REST_ENDPOINT, { | ||
| signal, | ||
| params: { | ||
| workspace_name: workspaceName, |
There was a problem hiding this comment.
No need of workspace_name here, it's okay for the dependency but no need in the endpoint
| const { data } = usePromptVersionsById( | ||
| { | ||
| promptId: prompt?.id || "", | ||
| page: 1, |
There was a problem hiding this comment.
So we won't have any sort of pagination in this component to load more in this page, right?
There was a problem hiding this comment.
@ferc we have the same data in Commits tab - there is a full flow of it with pagination
here we will show first 25 commits
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [name, description, template, promptCreateMutation.mutate]); |
There was a problem hiding this comment.
Maybe we should start destructuring the mutation like const { mutate: createPrompt } = usePromptCreateMutation(); so we can use it as dependency here
| <Button variant="outline" onClick={() => setOpenUseThisPrompt(true)}> | ||
| <Info className="mr-2 size-4" /> | ||
| Use this prompt | ||
| </Button> |
There was a problem hiding this comment.
Let's hide it until we have the snippets so we can merge without depending on the SDK
There was a problem hiding this comment.
Sorry for the misunderstanding about the workspace name in the project, since it's not sent the header inside the query it could be confusing that we still need the dependency in the query, therefore sent in the useQuery query key but not in the request body/parameters (automatically added in the header in a global state)
The rest looks great, nice work!
| @@ -216,7 +216,6 @@ const SideBar: React.FunctionComponent<SideBarProps> = ({ | |||
|
|
|||
| const { data: promptsData } = usePromptsList( | |||
| { | |||
| workspaceName, | |||
There was a problem hiding this comment.
Sorry for the misunderstanding here but you need the workspaceName as parameter, what you don't need is to send it in the request (it's automatically sent in the request header)
You have to pass it to the query because it should be part of the query key (otherwise on changing workspace name it'll display the cache from previous workspace)
| @@ -72,7 +72,6 @@ const PromptsPage: React.FunctionComponent = () => { | |||
| const [size, setSize] = useState(10); | |||
| const { data, isPending } = usePromptsList( | |||
| { | |||
| workspaceName, | |||
Details
Issues
Resolves #
Testing
Documentation